Drop projections to C++ and add cone program refinement#39
Conversation
| Q = sparse.bmat([ | ||
| [None, A.T, np.expand_dims(c, - 1)], | ||
| [-A, None, np.expand_dims(b, -1)], | ||
| [-np.expand_dims(c, -1).T, -np.expand_dims(b, -1).T, None] | ||
| ]) | ||
|
|
||
| D_proj_dual_cone = _diffcp.dprojection(v, cones_parsed, True) | ||
| if mode == "dense": | ||
| Q_dense = Q.todense() | ||
| M = _diffcp.M_dense(Q_dense, cones_parsed, u, v, w) | ||
| MT = M.T | ||
|
|
||
| pi_z = pi(z, cones) |
There was a problem hiding this comment.
The derivative callable could conceivably be called more than once for a single solve; in such cases, wouldn't recomputing Q, D_proj_dual_cone be wasteful (or have I missed something)? I'd prefer pre-computing these quantities and capturing them via lexical closure as before. Otherwise, lazy computation of these quantities would also be fine (i.e., they're computed on the first call to derivative, and re-used thereafter). I have the same comment for adjoint_derivative.
|
|
||
| pi_z = pi(z, cones) | ||
|
|
||
| M = _diffcp.M_operator(Q, cones_parsed, u, v, w) |
There was a problem hiding this comment.
Are these lines supposed to be in an else clause, accompanying line 301 (they overwrite M and MT from lines 303-34)?
| def pi(x, cones, dual=False): | ||
| """Projects x onto product of cones (or their duals) | ||
| Args: | ||
| x: NumPy array (with PSD data formatted in SCS convention) | ||
| cones: list of (cone name, size) | ||
| dual: whether to project onto the dual cone | ||
| Returns: | ||
| NumPy array that is the projection of `x` onto the (dual) cones | ||
| """ | ||
|
|
||
| cone_list_cpp = parse_cone_dict_cpp(cones) | ||
| return projection(x, cone_list_cpp, dual) |
There was a problem hiding this comment.
cool! might as well delete 'pi_python if it's no longer used.
|
Thanks for the PR @bamos! It's great that you've moved I did leave a couple comments --- in particular, I'm not sure I understand the motivation for moving the pre-computations for I haven't had a chance to review the refinement code. What do you think about splitting this PR into two separate ones --- one that moves |
Yes, sounds good. I'll close this PR and send in separate PRs for the projections and refinement separately
I noticed that not returning M back up to Python at all (and calling into the |
2 participants
Hi, here's the initial version of dropping the projections to C++ and adding the refinement from this paper. What do you all think?
pito call into C++ and moved the old Python version topi_pythontests.pyworks with the C++ projectionsderivativeandadjoint_derivativefunctions to better-compare the runtimes here. Something else I noticed is that we can improve the runtime a bit by not returningMback up to Python, so I created_solve_adjoint_derivative_lsqr. We can also do the same thing for the derivative, or can revert this if you all want to keep the pre-computations insolveprof.pyfrom this PR on the old code and here shows this PR doesn't slow down anything on this example and gives a slight performance boost on the adjoint derivative fromsolve_adjoint_derivative_lsqr. In the "old code" I also moved the pre-computations into thederivativeandadjoint_derivativefor a fair comparison here